Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[trimming] preserve custom views and $(AndroidHttpClientHandlerType) #8954

Merged
merged 6 commits into from
Jun 17, 2024

Conversation

jonathanpeppers
Copy link
Member

Fixes: #8797

Here are two cases TrimMode=full can break applications:

  • $(AndroidHttpClientHandlerType) set to a custom type

  • Custom views (Android .xml) that are not referenced in C# code

In the MarkJavaObjects trimmer step we can preserve both of these cases by:

  • Passing in $(AndroidHttpClientHandlerType), preserve the public, parameterless constructor of the type

  • Pass in $(_CustomViewMapFile), preserve IJavaObject types if they are found in the map file

Fixes: dotnet#8797

Here are two cases `TrimMode=full` can break applications:

* `$(AndroidHttpClientHandlerType)` set to a custom type

* Custom views (Android `.xml`) that are not referenced in C# code

In the `MarkJavaObjects` trimmer step we can preserve both of these
cases by:

* Passing in `$(AndroidHttpClientHandlerType)`, preserve the public,
  parameterless constructor of the type

* Pass in `$(_CustomViewMapFile)`, preserve `IJavaObject` types if
  they are found in the map file
@jonathanpeppers jonathanpeppers marked this pull request as ready for review May 17, 2024 13:44
markContext.RegisterMarkTypeAction (type => ProcessType (type));
}

bool IsActiveFor (AssemblyDefinition assembly)
{
return assembly.MainModule.HasTypeReference ("System.Net.Http.HttpMessageHandler") || assembly.MainModule.HasTypeReference ("Android.Util.IAttributeSet");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of this, is we don't want to iterate over every type in every assembly.

Is Android.Util.IAttributeSet required for any custom view?

https://github.com/xamarin/xamarin-android/blob/14b70ac32b2a9efefae127795e96ef73124c8557/tests/Mono.Android-Tests/Mono.Android-Test.Library/CustomTextView.cs#L9

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See:

IAttributeSet is used so that the View can obtain any XML attributes specified on the view. The above stack overflow answer has a good example of this:

<com.anjithsasindran.RectangleView
    app:radiusDimen="5dp"
    app:rectangleBackground="@color/yellow"
    app:circleBackground="@color/green" />

The AttributeSet is how the RectangleView constructor would lookup the values for @app:radiusDimen, @app:rectangleBackground, and @app:circleBackground.

I believe, but cannot quickly verify, that the (Context, IAttributeSet) constructor is always used/preferred by Android.

Comment on lines 15 to 16
// NOTE: ensure the C# compiler has a reference to the library
new Mono.Android_Test.Library.Foo ();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was another problem, when we have a @(ProjectReference) to the class library containing Mono.Android_Test.Library -- there was no C# code using types from the library. And so the C# compiler would omit the assembly reference, and the trimmer did not even receive it as input.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this (still?) a problem? #8904 updated things to look for "transitive assembly references" (see also).

If we need to add explicit type references to work around missing transitive assembly references, maybe we need to revisit #8904?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#8904 is unrelated.

The problem here is if you add a reference to a class library not using any types from it, and one of the AndroidResource .xml files uses a custom view. The C# compiler will ignore the assembly, but the AndroidResource files would still make it to the app.

I'm not sure if this would happen in practice/real customer projects.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I filed this for the future, and put it in comments:

@@ -48,6 +48,8 @@ public void ProcessAssembly (AssemblyDefinition assembly, string androidHttpClie
}

// Custom views in Android .xml files
if (!type.ImplementsIJavaObject (cache))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

set.Add (value);
}
return map;
return LoadCustomViewMapFile (mapFile);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we "register" mapFile? (And why didn't we do this before, given that the beginning of this method calls .GetRegisteredTaskObjectAssemblyLocal<…>(…)?)

var map  = LoadCustomViewMapFile (mapFile);
if (map != null) {
    engine.RegisterTaskObject (mapFile, map, RegisteredTaskObjectLifetime.Build, allowEarlyCollection: false);
}
return map;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh the code changed... ah cos of the linker can't use RegisterTaskObject

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry just actually read your comment (its early). The Registration of the file happens in SaveCustomViewMapFile.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the new method that doesn't support RegisterTaskObject is for the linker/trimmer step assembly. It doesn't have access to MSBuild APIs.

@jonathanpeppers jonathanpeppers merged commit bd95226 into dotnet:main Jun 17, 2024
47 checks passed
@jonathanpeppers jonathanpeppers deleted the issue8797take2 branch June 17, 2024 15:01
grendello added a commit that referenced this pull request Jun 21, 2024
* main: (26 commits)
  Make APK and shared library alignment configurable (#9046)
  [r8] update proguard rule to keep .NET runtime classes (#9044)
  Explicitly align to 4k (#9041)
  [trimming] preserve custom views and `$(AndroidHttpClientHandlerType)` (#8954)
  Ignore split configs when bundle config moves shared libraries to base.apk (#8987)
  Bump to dotnet/android-tools@1c09dcc (#9026)
  Bump to dotnet/java-interop@ccafbe6 (#9025)
  [Mono.Android-Tests] Fix repo URL in redirect tests (#9035)
  [ci] Update checkout path for nightly build (#9028)
  [ci] Fix android source path for MAUI test job (#9030)
  Link Code of Conduct (#9034)
  [ci] Update sdk-insertions trigger to manual only (#9029)
  Update java-interop and android-tools submodule mentions (#9023)
  LEGO: Merge pull request 9022
  [Xamarin.Android.Build.Tasks] fastdev works with aab files (#8990)
  Use new binutils URL (#9019)
  Localized file check-in by OneLocBuild Task: Build definition ID 17928: Build ID 9686669 (#9011)
  LEGO: Merge pull request 9015
  [api-merge] Update "constant" values to mirror latest API levels (#9004)
  [Mono.Android] Fix wrong value for `ApplicationExitInfoReason.Other` (#9003)
  ...
@github-actions github-actions bot locked and limited conversation to collaborators Jul 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When using TrimMode=link custom AndroidHttpClientHandlerType is not preserved
3 participants